Skip to content

feat(pluginpresets): pluginpreset controller resolves values from references - #2117

Open
k-fabryczny wants to merge 51 commits into
mainfrom
feat/v4-CEL-in-pp-1776
Open

feat(pluginpresets): pluginpreset controller resolves values from references#2117
k-fabryczny wants to merge 51 commits into
mainfrom
feat/v4-CEL-in-pp-1776

Conversation

@k-fabryczny

Copy link
Copy Markdown
Contributor

Description

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

#1776

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help
  • Separate ticket for tests # (issue/pr)

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added to documentation?

  • 📜 README.md
  • 🤝 Documentation pages updated
  • 🙅 no documentation needed
  • (if applicable) generated OpenAPI docs for CRD changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

Copilot AI review requested due to automatic review settings July 3, 2026 14:30
@k-fabryczny
k-fabryczny requested review from a team as code owners July 3, 2026 14:30
@github-actions github-actions Bot added documentation Improvements or additions to documentation feature helm-charts size/XXL labels Jul 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the PluginPreset controller to resolve valueFrom.ref references (to other PluginPresets) into concrete spec.optionValues[*].value entries on the generated Plugins, controlled via a new pluginPreset.integrationEnabled feature flag. It also updates Helm chart feature-flag wiring, adds extensive controller/e2e tests, and expands the PluginPreset documentation to cover cross-preset references.

Changes:

  • Add pluginPreset.integrationEnabled feature flag plumbed through features, controller wiring, and Helm charts.
  • Implement valueFrom.ref resolution in the PluginPreset controller (including selector-based fan-out and CEL extraction).
  • Add/extend unit and E2E scenarios and documentation for cross-PluginPreset references.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
internal/features/features.go Adds IsPresetIntegrationEnabled() and supporting struct field.
internal/features/features_test.go Extends feature-flag tests to cover preset integration flag.
internal/controller/plugin/suite_test.go Enables preset integration in controller test suite setup.
internal/controller/plugin/pluginpreset_values_resolver.go Implements reference resolution and CEL evaluation for valueFrom.ref.
internal/controller/plugin/pluginpreset_controller.go Adds IntegrationEnabled flag to reconciler struct.
internal/controller/plugin/pluginpreset_controller_test.go Adds multiple integration tests for cross-preset references.
e2e/pluginpreset/scenarios/selector_reference.go New E2E scenario: selector-based cross-preset reference.
e2e/pluginpreset/scenarios/cross_preset_reference.go New E2E scenario: name-based cross-preset reference.
e2e/pluginpreset/scenarios/cross_preset_reference_overrides.go New E2E scenario: references respect source ClusterOptionOverrides.
e2e/pluginpreset/scenarios/constants.go Adds constants for new E2E scenarios.
e2e/pluginpreset/e2e_test.go Registers new PluginPreset reference E2E scenarios.
docs/reference/components/pluginpreset.md Documents valueFrom.ref usage, syntax, and feature flags.
dev-env/dev.values.yaml Enables preset integration in dev values.
cmd/greenhouse/controllers.go Wires IsPresetIntegrationEnabled() into PluginPreset reconciler.
charts/manager/templates/manager/feature-flag.yaml Adds template docs + rendered value for preset integration flag.
charts/manager/templates/_helpers.tpl Adds helper for pluginPreset.integrationEnabled.
charts/greenhouse/values.yaml Adds default global.pluginPreset.integrationEnabled: false.
charts/greenhouse/ci/test-values.yaml Enables preset integration for chart CI values.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/controller/plugin/pluginpreset_values_resolver.go
Comment thread internal/controller/plugin/pluginpreset_values_resolver.go
Comment thread docs/reference/components/pluginpreset.md Outdated
Comment thread docs/reference/components/pluginpreset.md Outdated
k-fabryczny and others added 20 commits July 20, 2026 14:43
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
…esetToPluginOptionValues

Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
…, fix tests

Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
@k-fabryczny
k-fabryczny force-pushed the feat/v4-CEL-in-pp-1776 branch from 965b705 to 05fcb76 Compare July 20, 2026 14:44
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 45 out of 47 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
Comments suppressed due to low confidence (6)

internal/webhook/v1alpha1/pluginpreset_webhook.go:144

  • convertPresetToPluginOptionValues assigns ov.ValueFrom even when pv.ValueFrom.Secret is nil (e.g., when the preset uses valueFrom.ref). That produces PluginOptionValues with ValueFrom != nil but ValueFrom.Secret == nil, which can lead to nil-pointer panics in validatePluginOptionValues (it dereferences val.ValueFrom.Secret.* without nil checks) and/or invalid PluginOptionValues being validated.
    internal/controller/plugin/pluginpreset_values_resolver.go:59
  • When IntegrationEnabled is false, PluginPreset optionValues using valueFrom.ref will flow through without being resolved. In that case, resolvedValues is built from the preset's ValueFrom.Secret only, leaving a PluginOptionValue with ValueFrom != nil but Secret == nil (because the source was a ref). This violates the Plugin schema (PluginValueFromSource requires secret) and can cause Plugin creation/validation failures. Consider failing fast with a clear error when valueFrom.ref is present but integrationEnabled is disabled (similar to the expression feature gate).
    docs/reference/components/pluginpreset.md:186
  • This sentence says that when pluginPreset.expressionEvaluationEnabled is false, the resulting Plugins will keep the expression fields unchanged. However the controller rejects presets that contain expressions when the flag is disabled (and Plugins no longer have an expression field at all), so this behavior description is inaccurate.
CEL expression evaluation in PluginPresets requires the feature flag `pluginPreset.expressionEvaluationEnabled` to be set to `true` in the Greenhouse feature flags ConfigMap.

By default, this flag is `false` if it is unset or invalid, and expressions are not evaluated (the resulting Plugins will keep the `expression` fields unchanged).

charts/manager/templates/webhook/webhooks.yaml:242

  • This PR is scoped to PluginPreset value resolution, but this change removes the DELETE operation from the Cluster validating webhook. That alters admission behavior for cluster deletion and seems unrelated/risky unless explicitly intended. If this is not intentional, please restore DELETE here (or document why delete validation is no longer needed).
        operations:
          - CREATE
          - UPDATE
        resources:
          - clusters

internal/webhook/v1alpha1/pluginpreset_webhook_test.go:310

  • Two table entries covering expression-only and expression+value cases are commented out, leaving the expression validation behavior partially untested. Given expressions are a core part of PluginPreset behavior, it would be better to adapt and re-enable these cases as part of this PR (or add equivalent coverage elsewhere) so regressions are caught.
    internal/controller/plugin/pluginpreset_values_resolver.go:394
  • evaluateCELWithObject creates a new CEL environment on every evaluation. In selector-based references this happens once per matched PluginPreset, which can add noticeable CPU/alloc overhead. Consider constructing the env once (and ideally compiling the expression once) and reusing it across evaluations within a reconciliation.

Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 45 out of 47 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file
Comments suppressed due to low confidence (2)

internal/webhook/v1alpha1/plugin_webhook.go:336

  • The field path used for secret validation errors points to valueFrom.name / valueFrom.key, but the actual fields are nested under valueFrom.secret. This will surface confusing error locations to users.
    internal/controller/plugin/pluginpreset_values_resolver.go:324
  • When expression evaluation is disabled, referenced PluginPresets are converted without applying cluster-specific overrides. This means valueFrom.ref can read stale/default values instead of the overridden values for the target cluster.

Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core-apis documentation Improvements or additions to documentation feature helm-charts size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants